Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove need for config file for `atlas-densities cell-densities fit-average-densities command #67

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Feb 20, 2024

  • The updated command doesn't require a combine_markers_ccfv2_config.yaml
  • Instead, the command line has:
    --marker=pv:868:PATH/TO/pvalb.nrrd
    --marker=sst:1001:PATH/TO/SST.nrrd
    --marker=vip:77371835:PATH/TO/VIP.nrrd
    --marker=gad67:479:PATH/TO/gad1.nrrd
    --realigned-slices=atlas_densities/app/data/markers/realigned_slices_ccfv2.json
    --cell-density-standard-deviations=atlas_densities/app/data/measurements/std_cells.json
  • As discussed in: Hard-coded CCFv2 slice numbers for gene markers should be removed and set as an input parameter #66

…verage-densities` command

* The updated command doesn't require a combine_markers_ccfv2_config.yaml
* Instead, the command line has:
    --marker=pv:868:PATH/TO/pvalb.nrrd \
    --marker=sst:1001:PATH/TO/SST.nrrd \
    --marker=vip:77371835:PATH/TO/VIP.nrrd \
    --marker=gad67:479:PATH/TO/gad1.nrrd \
    --realigned-slices=atlas_densities/app/data/markers/realigned_slices_ccfv2.json \
    --cell-density-standard-deviations=atlas_densities/app/data/measurements/std_cells.json \
* As discussed in:
    #66
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@376c0a8). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #67   +/-   ##
=======================================
  Coverage        ?   97.78%           
=======================================
  Files           ?       22           
  Lines           ?     1447           
  Branches        ?        0           
=======================================
  Hits            ?     1415           
  Misses          ?       32           
  Partials        ?        0           
Flag Coverage Δ
pytest 97.78% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sebastien-PILUSO
Copy link

I think more changes are necessary to make the code really generic.
Instead of taking a realigned-slices=atlas_densities/app/data/markers/realigned_slices_ccfv2.json as input parameter, it should take all the JSON files attached to the 4 input genes consumed by this step of the pipeline, i.e. gene_vip-metadata.json, gene_gad67-metadata.json, gene_pv-metadata.json, and gene_sst-metadata.json. Those 4 files should then be converted and gathered into a realigned_slices_ccfv2.json according to Dimitri's code.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Feb 28, 2024

i.e. gene_vip-metadata.json, gene_gad67-metadata.json, gene_pv-metadata.json, and gene_sst-metadata.json

That can be done; I think that would obviate the need for the marker id parameter to --marker since the paths you provided previously were of the style `ccfv2/$marker-id-metadata.json.

Those 4 files should then be converted and gathered into a realigned_slices_ccfv2.json according to Dimitri's code.

Which code are you talking about?

@Sebastien-PILUSO
Copy link

That can be done; I think that would obviate the need for the marker id parameter to --marker since the paths you provided previously were of the style `ccfv2/$marker-id-metadata.json.

Maybe a folder path could be given? With expected filenames in it?

Which code are you talking about?

The code Dimitri shared on the atlas channel. See with @drodarie, he created such a convertion.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Feb 28, 2024

Maybe a folder path could be given? With expected filenames in it?

No, we should always make things explicit, so it's clear what is required.

The code Dimitri shared on the atlas channel. See with @drodarie, he created such a convertion.

I assume you mean this code: https://bluebrainproject.slack.com/archives/C016G4U2C8H/p1708092444378119
If we're just pulling values out of the .json files, I can change how that's handled to directly pull them out of $marker-id-metadata.json.. The other parts of that code (changing from npy to nrrd) are better handled as a separate step.

@drodarie
Copy link
Collaborator

drodarie commented Feb 28, 2024

I created a script to merge all the json results from Deep Atlas into a single json file that could be consumed by the atlas-densities pipeline. I agree with @Sebastien-PILUSO that having one json per experiment instead of adding an extra step is better.

However, IMO, I think this idea of removing configuration file goes in the wrong direction with respect to the future of the pipeline. Let's imagine you decide to change the composition rule for inhibitory neuron: now it is GAD2 = PV + SST + GENE4 or worse if you want to use T-types.
Then, you would have to refactor everything. Now don´t get me wrong, we would have to refactor a lot already but this is not going in the right direction.

Apart from that, I do not mind too much the changes proposed here. This is just a change of interface.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Feb 28, 2024

However, IMO, I think this idea of removing configuration file goes in the wrong direction [...]

The same information is conveyed in this, it just means that one doesn't need to change a file to make changes to which files are used; and it's one less file to keep track of.

If there are big changes, like you said, we'll have to refactor everything regardless.

@Sebastien-PILUSO
Copy link

So how about this correction of the code for adding the input json files as input?

@mgeplf
Copy link
Collaborator Author

mgeplf commented Mar 21, 2024

So how about this correction of the code for adding the input json files as input?

I have been away on vacation, and am currently catching up.

To confir, the final proposal for the command line is:

--marker=pv:PATH/TO/pv-metadata.json:PATH/TO/pvalb.nrrd
--marker=sst:PATH/TO/sst-metadata.json:PATH/TO/SST.nrrd
--marker=vip:PATH/TO/vip-metadata.json:PATH/TO/VIP.nrrd
--marker=gad67:PATH/TO/gad-metadata.json:PATH/TO/gad1.nrrd
--cell-density-standard-deviations=atlas_densities/app/data/measurements/std_cells.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants